SSLEngine memory leak fix, X509ExtendedTrustManager chain sorting fix, support for getExtendedKeyUsage()#289
Conversation
There was a problem hiding this comment.
Pull Request Overview
Fixes potential memory leaks in WolfSSLEngine caused by JNI global references not being reliably cleared when wrap()/unwrap()/beginHandshake() exited early due to exceptions. Ensures callback cleanup always runs and adds a regression test to detect leaks.
- Added session ticket and verify callback cleanup via unsetSSLCallbacks() and finally blocks in wrap(), unwrap(), and beginHandshake()
- Introduced try/finally guarding of core engine operations to guarantee cleanup
- Added memory leak regression test and included it in the test suite
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| src/java/com/wolfssl/provider/jsse/WolfSSLEngine.java | Ensures callback cleanup via try/finally; adds session ticket and verify callback unsetting |
| src/test/com/wolfssl/provider/jsse/test/WolfSSLJSSETestSuite.java | Registers new memory leak test in suite |
| src/test/com/wolfssl/provider/jsse/test/WolfSSLEngineMemoryLeakTest.java | Adds regression test that stresses engine allocation to detect JNI reference leaks |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
cconlon
force-pushed
the
wolfsslEngineMemCleanup
branch
2 times, most recently
from
a0bf843 to
42bd561
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
cconlon
force-pushed
the
wolfsslEngineMemCleanup
branch
3 times, most recently
from
7349703 to
65a6f32
Compare
cconlon
changed the title
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
cconlon
force-pushed
the
wolfsslEngineMemCleanup
branch
from
4d0beb0 to
195fb2a
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
…it test for regression
…b to WeakReferences, ease garbage collection ability
… wrap wolfSSL_X509_get_extended_key_usage() in WolfSSLCertificate.getExtendedKeyUsage()
…af cert detection in WolfSSLTrustX509 (X509ExtendedTrustManager)
…). Native wolfSSL_X509_STORE_GetCerts() direction changed between wolfSSL 5.7.0 and 5.8.0.
cconlon
force-pushed
the
wolfsslEngineMemCleanup
branch
from
bb70b9d to
c44f1c8
Compare
This PR fixes three issues and adds one feature:
Fix: SSLEngine Memory Leak
This PR fixes a potential memory leak in WolfSSLEngine by ensuring JNI callback cleanup and introducing WeakReference objects for
SSLSocketandSSLEngineheld inWolfSSLInternalVerifyCb.java.WolfSSLEngineinstances could leak memory due to JNI global references that prevent garbage collection. The references are for I/O callbacks (setIOWriteCtx, setIOReadCtx), session ticket callbacks, and verify callbacks, but were only cleaned up at the end of methods, where exceptions or early returns could bypass cleanup.For memory leak fixes, this PR:
unsetSSLCallbacks()wrap()andunwrap()main logic in try-finally blocks to make sureunsetSSLCallbacks()runs even on exceptions or early returnscheckAndInitSSLEngine()inside try block inbeginHandshake()SSLSocketandSSLEnginereferences inWolfSSLInternalVerifyCb.javato WeakReference objects, so they don't block garbage collection of the mainSSLEngine/SSLSocketobjects themselves.For testing, this PR:
WolfSSLEngineMemoryLeakTest) that verifies abandoned engines are garbage collected properly by measuring memory usage against set threshold.Fix: `WolfSSLX509StoreCtx.getCerts() cert chain ordering:
Between wolfSSL 5.7.0 and 5.8.0,
wolfSSL_X509_STORE_GetCerts()changed the direction of the certificate chain returned. Prior to 5.8.0, the chain was returned peer to root. Post 5.8.0 it was returned root to peer. Since Java/JSSE expects to get a chain that is sorted peer to root, this PR fixes behavior for the older pre-5.8.0 users. For pre-5.8.0 users, since the native chain is already peer to root, no manual reversing of order is needed in the JNI layer.Fix:
X509ExtendedTrustManagerleaf cert identification and sorting fix:Previously,
WolfSSLTrustX509.sortCertChainBySubjectIssuer()incorrectly assumed the leaf certificate was always at index 0 of the input chain. When certificate chains arrived with CA certificates before the server certificate, the sorting logic would discard the actual server cert and treat a CA cert as the peer cert.This caused Extended Key Usage validation failures in applications that validate serverAuth EKU, since CA certificates typically lack this extension.
Fixed by using RFC 5280 BasicConstraints extension to identify the leaf certificate:
getBasicConstraints()returns -1 for leaf/end-entity certs and >= 0 for CA certs. The leaf cert is now correctly identified regardless of its position in the input array.Added regression test
testExtendedKeyUsageWithLeafNotFirst()that validates EKU on the sorted chain to ensure the server cert (not a CA cert) is returned as the peer certificate.Implementation of
X509Certificate.getExtendedKeyUsage():This PR includes an implementation of
X509Certificate.getExtendedKeyUsage()to fix callers that may be calling that explicitly from their code or 3rd party framework. Unit tests added.wolfSSL_X509_get_extended_key_usage()underWolfSSLCertificate.getExtendedKeyUsage(). Unit tests added.WolfSSLSocketTest.testSNIMatchers()if native wolfSSL is lower than 5.7.2.ZD 20590